Skip to content

fix: [SDK-5154] stop mixing dashboard sender id with the shared FCM project - #2731

Closed
abdulraqeeb33 wants to merge 1 commit into
mainfrom
ar/sdk-5154-fcm-consistent-project-a09f
Closed

abdulraqeeb33 wants to merge 1 commit into
mainfrom
ar/sdk-5154-fcm-consistent-project-a09f

Conversation

@abdulraqeeb33

Copy link
Copy Markdown
Contributor

Description

One Line Summary

Register Android FCM tokens against one consistent Firebase project so we stop minting shared-project tokens that the dashboard reports as Invalid Google Project Number (-6) or Never Subscribed.

Details

Motivation

FirebaseMessaging.getToken() uses the Firebase project in FirebaseOptions (application id / project id / API key), not gcmSenderId. The SDK was initializing its own FirebaseApp with:

  • the customer's dashboard sender id
  • OneSignal's shared public project (onesignal-shared-public / 754795614042) when backend fcm params were missing or snapshotted empty in the PushRegistratorFCM constructor before android_params.js hydrated

That mixed config still produced a token (the shared project does not care about sender id), so a subscription was created. OneSignal then sends with the customer's FCM v1 credentials, the token's project does not match, and the dashboard shows Invalid Google Project Number (notification_types = -6) or Never Subscribed.

This is the SDK-5154 split of the google-services / consistent-project work from #2725, without the Firebase Installation ID / SDK-4946 path.

Scope

FCM registration now picks one consistent project, in this order:

  1. Host google-services.json — reuse the default FirebaseApp when it is complete and compatible with the dashboard sender (or when the dashboard sender is missing).
  2. Backend fcm params from android_params.js, read at registration time (not constructor snapshot). Rejected when they are actually the shared public project, or when the application id's project number does not match the dashboard sender.
  3. OneSignal shared project only when the dashboard sender is 754795614042. A customer sender is no longer paired with shared credentials.

If none of those form a consistent project, registration returns INVALID_FCM_SENDER_ID instead of uploading a token from the shared project.

Also:

  • Prefer a read-only FirebaseApp.getInstance() lookup before initializeApp(context).
  • Re-resolve on later getToken() calls so hydrated backend params can replace an earlier config (named FirebaseApp is deleted when credentials change).
  • No Installation ID / FirebaseMessaging.register() path (that stays on SDK-4946).

No public API changes.

Testing

Unit testing

  • FcmFirebaseConfigResolverTests covers host match/mismatch, missing dashboard sender, backend consistency (including mixed application id / shared project classified as backend), and refusing to mix a customer sender with the shared project.
  • PushRegistratorFCMTests covers host reuse, backend params (including after construction and after a first registration), shared-project refusal for customer senders, INVALID_FCM_SENDER_ID from registerForPush, and hostDefaultFirebaseApp lookup.

:OneSignal:notifications:testDebugUnitTest --tests com.onesignal.notifications.internal.registration.impl.*, Spotless, and Detekt pass locally.

Manual testing

Not run on a device in this environment. To verify on a device:

  1. App with google-services.json whose sender matches the OneSignal dashboard — logs should say registration is using that project, and a push should arrive.
  2. App without google-services.json but with complete Android FCM config on the dashboard — registration should use backend fcm params after android_params.js hydrates (including first session).
  3. App with a customer dashboard sender and neither google-services nor backend fcm params — should not subscribe via onesignal-shared-public; status should be Invalid Google Project Number rather than a silent shared-project token.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing — FCM registration uses a consistent Firebase project
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible — see Manual testing

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item
Open in Web Open in Cursor 

…roject

FCM getToken() uses the Firebase project in FirebaseOptions, not gcmSenderId.
Pairing a customer dashboard sender with onesignal-shared-public minted a token
the customer's FCM v1 credentials cannot send to, which showed up as Invalid
Google Project Number (-6) or Never Subscribed.

Prefer google-services.json, then complete backend fcm params, and only use the
shared project when the dashboard sender is that project's own sender.

Co-authored-by: abdulraqeeb33 <abdulraqeeb33@users.noreply.github.com>
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

  • FcmFirebaseConfigResolver.kt: 51/51 touched executable lines (100.0%) (164 touched lines in diff)
  • PushRegistratorAbstractGoogle.kt: 10/10 touched executable lines (100.0%) (16 touched lines in diff)
  • PushRegistratorFCM.kt: 88/93 touched executable lines (94.6%) (198 touched lines in diff)

Overall (aggregate gate)

149/154 touched executable lines covered (96.8% — requires ≥ 80%)

📥 View workflow run

@abdulraqeeb33
abdulraqeeb33 marked this pull request as ready for review September 2, 2026 18:03
@abdulraqeeb33
abdulraqeeb33 requested a review from a team as a code owner September 2, 2026 18:03

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-model review

Claude Opus 5, GPT 5.6 Sol, Cursor Grok 4.6. Diff +1084/−47 (5 Kotlin files). No generated/lock/vendor/snapshots.

The consistent-project goal is right. Treating android_sender_id as a hard gate can recreate the same -6 this PR is fixing.

Act on

  1. Do not discard a complete customer project because the dashboard sender disagrees (Opus + Grok). getToken() uses project fields, not gcmSenderId. If android_sender_id is still 754795614042 and backend/host fcm is the customer project, compatibleWithDashboard fails and useShared wins — shared token again. Inverse: a customer sender plus a different-project backend fcm is rejected entirely → hard -6. Prefer any complete non-shared host/backend; use shared only when no customer project exists.
  2. isShared is only applied to backend (3/3). A missing dashboard sender plus a host default that is onesignal-shared-public still mints a shared token — the case the comments say they refuse.

Consider

  • Token fetch is outside firebaseLock; start/HYDRATE can delete() the named app mid-Tasks.await (3/3). New vs old code.
  • -6 is not retryable. A miss on empty persisted fcmParams before HYDRATE can unsubscribe until params arrive (Grok).
  • hostDefaultFirebaseApp still calls initializeApp(context) when the host deferred Firebase init (Opus, Grok).
  • Unparseable app_id is treated as compatible (Opus, Sol).

Noted / dismissed

  • Switching named app → host default rotates tokens (Opus; release-note worthy, not a logic bug).
  • Shared API key Base64 is pre-existing; this PR reduces when it is used.
  • missingProjectMessage / host-mismatch log can blame the wrong source (nits).

No tests for: dashboard=shared + customer backend; host shared + null dashboard; concurrent getToken + delete().

Open in Web View Automation 

Sent by Cursor Automation: PR Reviews

Comment on lines +112 to +120
val matchingDefaultApp =
defaultApp?.takeIf { it.isComplete && compatibleWithDashboard(it, dashboard) }
val matchingBackend =
backend?.takeIf {
it.isComplete &&
!FcmSharedProject.isShared(it) &&
compatibleWithDashboard(it, dashboard)
}
val useShared = dashboard == FcmSharedProject.SENDER_ID && sharedDefault.isComplete

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Act on (Opus + Grok): dashboard sender is a hard gate, but getToken() ignores gcmSenderId.

  • Dashboard still 754795614042 + complete customer backend/host → compatibleWithDashboard fails, then useShared is true → shared-project token (the -6 this PR fixes). Old code minted from the customer fcm fields.
  • Customer dashboard + different-project backend fcm → both sources rejected → hard -6.

Prefer any complete non-shared host/backend. Use shared only when no customer project exists; log sender mismatches. Also apply isShared to defaultApp — missing dashboard + shared host currently passes (3/3).

Comment on lines 80 to 83
override suspend fun getToken(senderId: String): String {
initFirebaseApp(senderId)
return getTokenWithClassFirebaseMessaging()
val app = initFirebaseApp(senderId)
return getTokenWithClassFirebaseMessaging(app)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider (3/3): firebaseLock covers resolve/delete/init only. Tasks.await runs after the lock is released. DeviceRegistrationListener fires retrievePushToken from start(), HYDRATE, and permission change on IO — so one caller can current.delete() the named app another is still awaiting. Old code never deleted. Hold the lock through token fetch, or refcount in-flight users before delete().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants